-
Notifications
You must be signed in to change notification settings - Fork 667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTTP/2 #5659
Conversation
@ogoffart, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jturcotte, @ckamm and @krnowak to be potential reviewers. |
I have not tried this on any real http2 server yet. We anyway will have to wait for Qt 5.9 |
Another problem i foresee is that this might cause the sycning "long-running" job to make the other short lived control job queued and timeout. I'm thinking about the quota check for the UI, or the connection validator, or the job to expend the selective sync view. Currently, without HTTP2, we only limit the transfer of long running (big PUT or GET) jobs to 3. But Qt keeps 6 TCP connection open. So the short selective sync jobs happen in parallel and are still finishing well on time. However, with HTTP2, there is only one connection and the big job might take the whole bandwidth of this connection, meaning that the smaller jobs would be queued and will not finish in time to have a responsible UI. The solution would be to use higher priority for such jobs. But the Qt API does not support setting a priority on the request, as far as i know. |
As mentioned on IRC, we need to set maintenance jobs to HighPriority (QNetworkRequest property). |
We talked to the developer of HTTP2 in Qt, he did not check if certain PUT requests can stall each other, e.g. one request gets all the send window and other requests don't progress. |
if (max) | ||
return max; | ||
if (_account->isHttp2Supported()) | ||
return 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only in download or upload also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That number is for smaller jobs (mkcol, small uploads, small downloads, move, delete)
the maxium amount of longer jobs is still 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But anyways, it might need some tests probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
20 sounds scary :-) let's see if the servers can handle it.. @DeepDiver1975 @PVince81 @butonic
272c3ee
to
db22fc5
Compare
Using Qt 5.9 beta, I did some basic tests against an HTTP/2 enabled server, and everything seemed to work. |
I will happily test it again looking on performance if we would decide to integrate http2 in the client officialy @labkode @moscicki @DeepDiver1975. I can probably look into number of parallel files both for EOS/Standard OC/S3? |
@mrow4a Yes, we integrate HTTP2 in the client officially, and this pull request does it. Now that Qt 5.9 is released, we could already integrate this patch. |
Was #5659 (comment) adressed? |
We set a low priority on the PUT and GET jobs:
|
[Conflicts resolved] Any objections merging this to master now? |
src/libsync/accessmanager.cpp
Outdated
@@ -79,6 +79,15 @@ QNetworkReply *AccessManager::createRequest(QNetworkAccessManager::Operation op, | |||
if (verb == "PROPFIND") { | |||
newRequest.setHeader(QNetworkRequest::ContentTypeHeader, QLatin1String("text/xml; charset=utf-8")); | |||
} | |||
|
|||
#if QT_VERSION >= QT_VERSION_CHECK(5, 9, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't it 5.9.1 with all necessary bugfixes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are necessary bugfixes in 5.9.1.
@ogoffart are the upload/download propagator low priorities introduced here also applicable for non-HTTP/2 requests? No questions/objections apart from that - just tested it on macOS with Qt 5.9.1 and everything seem to be smooth. We might want to start setting a plan to have a stable Qt 5.9 toolchain on the build system. |
@SamuAlfageme Yes, this also apply to Non-HTTP/2 requests, that should not change much actually for we never have more than 6 requests at the same time normally (and if we have, this is actually a good change) |
We need Qt 5.9 for HTTP2 because, even if Qt 5.8 already has support for it, there is some critical bug in the HTTP2 implementation which make it unusable [ https://codereview.qt-project.org/186050 and https://codereview.qt-project.org/186066 ] When using HTTP2, we can use many more parallel network request, this is especially good for small file handling Lower the priority of the GET and PUT propagation jobs, so the quota or selective sync ui PROPFIND will not be blocked by them
Qt would otherwise still try to do HTTP/2 connection even over "http://". And that does not work with server that does not support it
We need Qt 5.9 for HTTP2 because, even if Qt 5.8 already has support
for it, there is some critical bug in the HTTP2 implementation which
make it unusable [ https://codereview.qt-project.org/186050 and
https://codereview.qt-project.org/186066 ]
When using HTTP2, we can use many more parallel network request, this
is especially good for small file handling